Skip to content

bun test --files-from: skip a UTF-8 byte order mark at the start of the list - #48257

Open
robobun wants to merge 2 commits into
mainfrom
robobun/8b07d276/files-from-skip-utf8-bom
Open

robobun wants to merge 2 commits into
mainfrom
robobun/8b07d276/files-from-skip-utf8-bom

Conversation

@robobun

@robobun robobun commented Nov 8, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • bun test --files-from skips the first file of a list that starts with a UTF-8 byte order mark (EF BB BF), and exits 0. A list of that one line prints Test filter "./\uFEFF./a.test.ts" had no matches.
  • append_test_files_from (src/runtime/cli/Arguments.rs:67) splits the raw bytes, so the first path is ./<mark>./a.test.ts. bun test passes over a missing path when other paths exist.

Fix

  • Cut the mark with strings::without_utf8_bom before the split. Only the first three bytes of each list are a mark.
  • Correct because this function alone produces list paths, and it cuts before it builds a path.
  • Verified: test/cli/test/files-from.test.ts: 6 of 7 new tests fail on main 2596ac5, all pass with the change. Also ran the file with --aot child runs.
  • Self-reviewed: 8 concerns raised, 6 addressed (see Notes).

Background

Downsides

  • An unmarked list whose first line is a bare name that starts with U+FEFF loses that file. Write ./ before it.
  • A list with only the mark, beside a path on the command line, exited 0. It now exits 1 with lists no test files.
  • An unmarked list runs 4 more instructions, with no new allocation or syscall. arguments::parse grows 48 bytes.
Notes

Repro

for t in a b c; do printf 'import { test, expect } from "bun:test";\ntest("in %s", () => { expect(1).toBe(1); });\n' $t > $t.test.ts; done
printf '\xef\xbb\xbf./a.test.ts\n./b.test.ts\n./c.test.ts\n' > list.txt
bun test --files-from=./list.txt
list main 2596ac5 this PR
mark, ./a, ./b, ./c (also bare names, also CRLF) b, c run, exit 0 a, b, c run, exit 0
mark and one line Test filter "./\uFEFF./a.test.ts" had no matches, exit 1 a runs, exit 0
only the mark Test filter "./\uFEFF" had no matches, exit 1 lists no test files, exit 1
no mark, LF or CRLF a, b, c run a, b, c run

I ran the repro on a debug build of main, on a local release build of main, and on the canary build 1.4.4-canary.1+2596ac5d3.

Tests

  • --files-from skips a UTF-8 byte order mark at the start of the list: four lists (paths, names with CRLF, an absolute first path, two lists). Each has an unlisted file that must not run.
  • a list that holds only a byte order mark lists no test files.
  • U+FEFF after the start of the list is a character of the name: two lists. The first holds the mark, a.test.js, then a file whose name really starts with U+FEFF. Both run. The second has no mark and lists that file with ./ before the name. It passes on main too. These tests state the rule that only byte 0 is a mark, and hold the ./ form.
  • The tests run one after another. In a debug build, six bun test child processes at once take more than 5 seconds each.
  • test/aot/fuzz/grids/runner-suites-flags.ts runs this file with --aot child runs. Its files-from share passes with a release build of this PR and fails with a release build of main.
  • I ran Linux x64 only. By a read of the Windows and macOS path code, no step changes a U+FEFF in a path, and test/cli/run/run-unicode.test.ts already uses U+200B in paths on all platforms.

Measurements

Two local release builds from one toolchain and one build directory: main 2596ac5, and the same tree with this change.

  • Instructions, unmarked list of 3,052 lines, a run that stops after the list is parsed (bun test unit --files-from=big.txt): medians 926,136 and 926,138 instructions:u over 5 interleaved runs. The spread within one build is 7 to 13, so the delta of 2 is below the noise.
  • Added code (objdump of bun_runtime::cli::arguments::parse, where the function is inlined): 10 instructions (4 compares, 4 branches, 2 adds). An unmarked list runs 4 of them.
  • Marked list of 3,052 lines: 926,283 to 926,145 instructions. The first line no longer gets the ./ prefix.
  • Size: size reports text 137,931,057, data 124,128, bss 1,861,880 for both builds. nm -S: arguments::parse 25,874 to 25,922 bytes. It is the only one of 191,938 symbols whose size differs. No symbol is added.
  • Allocator calls in the parse-only run (gdb hit counts): mi_malloc 3,076, mi_realloc 12, mi_zalloc 3, on both builds, for the unmarked and the marked list. parse has 462 call instructions in both builds.
  • Syscalls (gdb catch syscall): the marked 3-file list makes 1 openat with the mark in the path on main and 0 with this PR. openat on a.test.ts: 0 to 3. Total openat: 25 to 27, equal to the unmarked control. The parse-only run makes 95 syscalls on both builds.
  • A name that really starts with U+FEFF, in an unmarked list. Bare on line 1: main runs that file, this PR runs x.test.ts if it exists, else passes the line over (2 files to 1, exit 0). With ./ on line 1: no change. Bare on line 2: no change.
  • Test time: 7 more bun test child processes in files-from.test.ts (8 before), about 2 seconds in a debug build.
  • --shard membership changes for a marked list, because the list has one more file. With four files, shard 1/2 ran b and d on main. It runs a and c with this PR.

Not changed here

Self-review concerns not taken

  • Write the mark as a Buffer of EF BB BF. The tests fail without the fix, which shows the bytes are on disk.
  • Add the mark plus CRLF as a second empty list. The rule is the same, and it adds a child process.

…he list

A list that starts with the bytes EF BB BF lost its first file. The mark
stayed in front of the first path, so that path named no file and the run
passed it over. With other paths in the list, the exit code was 0.

Cut the mark before the list is split into lines. `bun pm pack` does the
same for .npmignore and .gitignore.
@coderabbitai

coderabbitai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 19 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 2596ac5 and abc57e2.

Files selected for processing: 2

  • src/runtime/cli/Arguments.rs
  • test/cli/test/files-from.test.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@github-actions github-actions Bot added the claude label Nov 8, 2026
@robobun

robobun commented Nov 8, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 3:05 PM PT - Nov 8th, 2026

❌ @robobun, your commit abc57e2 has some failures in Build #138829 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 48257

That installs a local version of the PR into your bun-48257 executable, so you can run:

bun-48257 --bun

@robobun

robobun commented Nov 8, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. The diff is green on every CI lane that ran.

CI at abc57e2 (build 138829, finished 23:05 UTC on 8 November). The build is marked failed for one reason: the 4 macOS x64 test shards never ran. They waited 5 hours for an agent, expired 4 times, and the automatic retries ran out. No job failed on a test: 184 of 188 jobs passed. test/cli/test/files-from.test.ts passed 14 of 14 on Windows 2019 x64, Windows 11 aarch64 and macOS aarch64. All Linux test shards passed. The macOS x64 test queue was overloaded for all builds at that time (263 jobs waited for 9 agents), so a new run was likely to end the same way.

How I reproduced it. On main 2596ac5 (a debug build, a local release build, and the canary build 1.4.4-canary.1+2596ac5d3), a list that starts with the bytes EF BB BF runs only b.test.ts and c.test.ts of three listed files, and exits 0:

for t in a b c; do printf 'import { test, expect } from "bun:test";\ntest("in %s", () => { expect(1).toBe(1); });\n' $t > $t.test.ts; done
printf '\xef\xbb\xbf./a.test.ts\n./b.test.ts\n./c.test.ts\n' > list.txt
bun test --files-from=./list.txt

With this change the same list runs a, b and c. Of the 7 new tests in test/cli/test/files-from.test.ts, 6 fail on main. All 14 tests of the file pass with the change.

PR: #48257

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/runtime/cli/Arguments.rs Resolved
…-from list

An unmarked list whose first line is a bare name that starts with U+FEFF
reads as a marked list. The way to list such a file is `./` before the
name. Hold that form in a test, beside the later-line case.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants